Skip to content

Userlib: Fix endianness in request id calculation#319

Merged
chenyan-dfinity merged 4 commits intomasterfrom
joachim/userlib-request-id
Jan 16, 2020
Merged

Userlib: Fix endianness in request id calculation#319
chenyan-dfinity merged 4 commits intomasterfrom
joachim/userlib-request-id

Conversation

@nomeata
Copy link
Contributor

@nomeata nomeata commented Jan 15, 2020

No description provided.

this is the equivalent of what #303 did to `dfx`: When CBOR-encoding a
canisterid for submission in a request, we need to make sure it is
little endian.

We do the change in
```
class CanisterIdEncoder implements CborEncoder<CanisterId> { …
```
where it belongs: As close to the HTTP interface we can.

This means that once we migrate to
https://github.com/dfinity-lab/dfinity/pull/2286
we will adjust this code to switch to CBOR blob, and the work-around can
be dropped here. Note in the code about that.

Also cleans up `fromText` to be more robust: It will _only_ accept `ic:`
textual representation, and _always_ strip the checksum.

With this, I can
 * build and install Linkedup
 * copy the `ic:` url printed by `dfx` upon installation
 * use that in the url (e.g. `http://127.0.0.1:8000/?canisterId=ic:72028DEDFB546FA89A`)
 * see it load the assets
@nomeata nomeata force-pushed the joachim/userlib-request-id branch 2 times, most recently from 42a1ce3 to 3b4ca63 Compare January 15, 2020 15:57
@nomeata
Copy link
Contributor Author

nomeata commented Jan 15, 2020

(doesn’t cut it yet)

@nomeata
Copy link
Contributor Author

nomeata commented Jan 15, 2020

Yes, this fixes it (Remember to Shift-Reload the browser…)

@nomeata nomeata marked this pull request as ready for review January 15, 2020 16:18
@nomeata nomeata requested a review from a team as a code owner January 15, 2020 16:18
@chenyan-dfinity
Copy link
Contributor

So #317 fixed the retrieving asset problem, and this PR fixes the whole thing?

@ggreif
Copy link
Contributor

ggreif commented Jan 15, 2020

So #317 fixed the retrieving asset problem, and this PR fixes the whole thing?

That's what I understood from @nomeata. https://dfinity.slack.com/archives/CGA566TPV/p1579105147298200?thread_ts=1579080257.280600&cid=CGA566TPV

@chenyan-dfinity
Copy link
Contributor

hmm...i still get the canister does not exist error

@ggreif
Copy link
Contributor

ggreif commented Jan 15, 2020

hmm...i still get the canister does not exist error

Did you shift - Reload in the browser?

What is your testcase?

  • manual testing
  • e2e test

@chenyan-dfinity
Copy link
Contributor

Yes, manual testing the default greet function. I am using a new browser, so it's not cached.

@ggreif ggreif changed the title Userlib: Fix endiannessin request id calculation Userlib: Fix endianness in request id calculation Jan 15, 2020
Copy link
Contributor

@chenyan-dfinity chenyan-dfinity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, this indeed works on the default app. I wasn't rebuilding a new version of dfx, so nix is fetching an old version js lib.

I still cannot get my matrix multiply example to work. Let's merge to master first.

@chenyan-dfinity chenyan-dfinity merged commit 989c5b5 into master Jan 16, 2020
@chenyan-dfinity chenyan-dfinity deleted the joachim/userlib-request-id branch January 16, 2020 00:02
@nomeata
Copy link
Contributor Author

nomeata commented Jan 16, 2020

ok, this indeed works on the default app. I wasn't rebuilding a new version of dfx, so nix is fetching an old version js lib.

I wonder how many developer days we have lost due to ~/.dfinity/cache being stale, or not using the right version of dfx, or the browser cache caching the user lib.

@nomeata nomeata mentioned this pull request Jan 16, 2020
@chenyan-dfinity
Copy link
Contributor

ok, this indeed works on the default app. I wasn't rebuilding a new version of dfx, so nix is fetching an old version js lib.

I wonder how many developer days we have lost due to ~/.dfinity/cache being stale, or not using the right version of dfx, or the browser cache caching the user lib.

@hansl This is indeed very painful. Any way we can design this better? At least, if I'm in nix-shell, I should always get the cache from my repo....

@hansl
Copy link
Contributor

hansl commented Jan 24, 2020

Agreed. Shouldn't be a big change.

@hansl
Copy link
Contributor

hansl commented Jan 24, 2020

@nomeata

I wonder how many developer days we have lost due to ~/.dfinity/cache being stale, or not using the right version of dfx, or the browser cache caching the user lib.

None. End-developers (users?) will not see cache being stale (because we don't release twice with the same version). This is only a problem when developing dfx itself, and could be fixed with a simple change.

Version numbers save lives!

@nomeata
Copy link
Contributor Author

nomeata commented Jan 24, 2020

I’d say content-addressed caches save lifes…

What’s the “simple change” you envision?

@hansl
Copy link
Contributor

hansl commented Jan 24, 2020

If your versions don't change when your content changes, I feel bad for you son ;)

Simple change -> If dirty version (ie. built from sources), invalidate cache.

@nomeata
Copy link
Contributor Author

nomeata commented Jan 24, 2020

Hmm, if the performance hit of extracing the data upon every invocation to dfx is not to bad, and if building from source by default has no version number (of one marked as dirty), then yes, that would work.

@hansl
Copy link
Contributor

hansl commented Jan 24, 2020

Extracting cache is ~500 msec, but isn't necessary for all commands (e.g. dfx canister call doesn't require the cache).

@nomeata
Copy link
Contributor Author

nomeata commented Jan 24, 2020

If your versions don't change when your content changes, I feel bad for you son ;)

Then feel bad for everyone trying to use dfx from source. Because if I build dfx, I always get the same version number.

@hansl
Copy link
Contributor

hansl commented Jan 24, 2020

Yup, I feel bad for us; that's why I'm doing that simple change. My original point though was that no users would be affected.

@nomeata
Copy link
Contributor Author

nomeata commented Jan 25, 2020

Until users start building from source :-)

eftychis added a commit that referenced this pull request Jan 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants